Reintroduce observable state,props,context in mobx-react #3863

jzhan-canva · 2024-04-26T04:32:42Z

As requested in #3858
This PR re-introduce the observable state, props, context. It is proven to be safe to restore this feature as the bug caused by setter is now gone.

Regarding two requests from @urugator

render callbacks accessing this.props/state/context should not force updates during render

This does seem to happen according to exp.test.js

shallow check should be called only once per component update

This is now solved by storing shouldComponentUpdate result in a WeakSet, so we don't re compare in setter

Next: I solved the class component tearing by wrapping class component in a function component, then utilise useSyncExternalStore. I decided to keep it in separate PR after this is merged

Code change checklist

Added/updated unit tests
Updated /docs. For new functionality, at least API.md should be updated
Verified that there is no significant performance drop (yarn mobx test:performance)

changeset-bot · 2024-04-26T04:32:45Z

🦋 Changeset detected

Latest commit: 3a8c951

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
mobx-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jzhan-canva · 2024-05-07T04:57:02Z

Hi @urugator what do you think about this PR? do you think it's safe to reintroduce the observability?

urugator · 2024-05-07T10:01:18Z

While the global state version was the main motivation for this change, we had other reasons, some conceptual, some technical. I will share parts of our discussion:

(@urugator) Btw the setup described above - accessing this.props of parent as part of callback send to descendant - causes double renders (on current stable versions), here is a test to demonstrate the problem: https://github.com/urugator/mobx/blob/exp/packages/mobx-react/__tests__/exp.test.tsx#L27

(@urugator) Imo we should not let a change of props of current component to cause forceUpdate on other components. It's just a no-no from React perspective. It's doable by making isUpdating flag global, shared by all components (the flag that prevents notifying component itself, preventing cycle). So a reactive props can only invalidate user defined computeds/autoruns - usually some local computations in class component - this was the original motivation for reactive props.
It will break the linked test case, but that would be expected React behavior without observables - if you replace observer with memo, it would fail as well. Note there are no other observables in the test except that internal reactive props.
Just to clarify again, I am talking about current behavior, the test above runs against the current version, not the new one.

(@urugator) In short, current existing behavior is flawed:
It causes excessive renders when using render callbacks.
At the same time it allows sending referentially stable callbacks without using memoization, which probably wasn't ever intended as a feature, but people can rely on this without knowing.

(@mweststrate) I've been a bit digging in both directions, and I think we're starting to pile patches upon patches for this reactive state/props concepts, without very little benefit; and I'm inclined to instead just drop that entire concept. It has the benefit that we become much better citizens and stop hacking into React it self. Stopping that behavior basically breaks two things 1) computed values depending on props / state won't work anymore. The fix there is simple; remove the @computed annotation (and take the perf hit). The trickier problem is autoruns / reactions. I think (hope) that is a rare case. But anyway, it will be more consistent with how observer function components work, where you have to be aware as well that props / state are non reactive, and hence have to combine useEffect and reactions in the right way to make it work. So it requires some good eduction on the pitfalls and a full new major, but I think it leaves us in a much more sustainable situation.

Another problem with the implementation is that shallow equality check runs twice for props/state/context on every component update - in each setter to decide whether atom should reportChanged and then again in shouldComponentUpdate.
It's also unfortunate that the implemention affects performance of all observer components regardless of whether they need it or not and I assume majority of observers don't need it. At the same time, I think that making it configurable would lead to more confusion.

If we are to reintroduce observable props (the decision is on @mweststrate), I would at least like to see the above problems addressed:

render callbacks accessing this.props/state/context should not force updates during render
shallow check should be called only once per component update

Both can be probably workarounded, but it's some extra work and unknown problems may emerge (getting back to pilling patches point).

jzhan-canva · 2024-05-08T11:16:40Z

Hi @urugator thanks for the informative reply.

render callbacks accessing this.props/state/context should not force updates during render

I tested your linked test case on my branch, it seems to me that Child does render 2 times in total which is desired as per comment, I'm not sure how to reproduce the unnecessary render (3 times) you described. In that example, Parent passes renderCallback method to Child, this won't trigger a re-render as the reference remain the same

shallow check should be called only once per component update

I agree there are some performance benefits from achieving this, I spent few hours and I couldn't achieve it in a good manner. However, (this is hacking React) that react reconciler always call shouldComponentUpdate first, then set instance.props after it, followed by setting state and context. Which means we could memorised the most recent shallowEqual call (I don't recommend this)
To us right now, removing @computed has a much bigger performance regression than extra shallowEqual

We would really appreciate it if we could make observable props behind a config, at least people like us can opt-in this feature

jzhan-canva · 2024-05-23T11:45:59Z

hello @urugator

I made some progress

I made several changes to ObserverClass

as discussed above, reintroducing observable props state context
solved extra shallowEqual calls. because I stop using forceUpdate API (I will explain why I stop using this API in no.4), so shouldComponentUpdate is always called. During shouldComponentUpdate, there is a weakSet to store what have been compared, so in setter we don't need to re-compare them
also in shouldComponentUpdate, I also shallow compare the state because that's how react is doing
wrap the class component in a function component, so we can use useSyncExternalStore to schedule the update in SyncLane, this solves the tearing between class observer and function observer

however no.4 brings few tradeoffs

the static properties of class component are no longer accessible, this can be solved by re-injecting the static properties into new function component (hacky)
ObserverClass itself can't detect "Redeclaring an existing observer component as an observer should throw", we need to move the logic to observer.tsx
observer class can't be extended (as it's a function now)
observer can only be used on Component, not PureComponent, this isn't a big issue as we always do shallow compare

jzhan-canva · 2024-05-23T12:06:43Z

~~The main logic change is to remove admin.isUpdating and add admin.pendingStateVersion and admin.stateVersion~~

when admin.pendingStateVersion is assigned a new symbol value, i.e. admin.pendingStateVersion !== admin.stateVersion. it means an update is scheduled or in progress (either scheduled by react or by admin.scheduleUpdate)

during the code you can see several admin.pendingStateVersion = Symbol() to make sure we are not rescheduling redundant updates. only at the end of render function it will assign admin.stateVersion = admin.pendingStateVersion

jzhan-canva · 2024-05-23T12:13:57Z

packages/mobx-react/__tests__/observer.test.tsx

@@ -513,7 +509,7 @@ describe("should render component even if setState called with exactly the same
        act(() => clickableDiv.click())
        act(() => clickableDiv.click())

-        expect(renderCount).toBe(3)


state is shallow compared now, renderCount should be 2

jzhan-canva · 2024-06-04T07:36:04Z

Hi @urugator @mweststrate I'm keen to hear you thoughts, I understand wrapping class component with function component is not a easy decision, but this is the only way to schedule a SyncLane update without hacking react internal logic (I have few solutions in this direction)

jzhan-canva · 2024-06-04T09:14:23Z

~~Close this PR as I need to reorganise the changes~~
reopened

mweststrate · 2024-06-04T17:43:16Z

Thanks for the extensive work here! I'll need to study more carefully the effects of this change, but I'm quite swamped atm. Feel free to ping if I didn't circle back in ~2 weeks!

urugator · 2024-06-04T19:39:12Z

packages/mobx-react/__tests__/observer.test.tsx

-    unmount()
-})
-
-test(`Observable props/state/context workaround`, () => {


Preserve this test.

urugator · 2024-06-04T19:39:20Z

packages/mobx-react/__tests__/observer.test.tsx

@@ -1115,189 +1125,12 @@ test(`Observable changes in componenWillUnmount don't cause any warnings or erro
    consoleWarnSpy.mockRestore()
 })

-test(`Observable prop workaround`, () => {


Preserve this test.

urugator · 2024-06-04T20:00:37Z

packages/mobx-react/__tests__/exp.test.tsx

+        container.querySelector("div")?.click()
+    })
+    expect(container).toHaveTextContent("1")
+    expect(renderCount).toBe(2)


It should be 1. The actual number isn't super relevant, it can depend on some React batching implementation details.
The point is that we must not issue an update on ANY component, when a component is already updating.
We use isUpdating flag to prevent updating self, but we must avoid calling forceUpdate also on other components - in this case on Child component, which is subscribed to Parent.props, because we leaked Parent.props to Child via callback.
It's like calling childRef.forceUpdate() during Parent's render. We must not do that. It's an oversight in the original implementation. Once component is updating (Parent recieved new props) the only proper way to propagate change to Child is via props. This would normally happen if you would create the callback during render.
The test was supposed to demonstrate that while this behavior is not correct, there is this unintended "feature" that you can send the same callback ref and still get the update (which is not supposed to happen with memo).
So to fix this, we probably just need to make isUpdating flag global.

Another way to put it is, that props must be observable for every derivation, except for observer derivation - that is any user defined computed/reaction/autorun.

Hope it's a bit clearer now.

Maybe a bit less obvious example of usage that will break once it's fixed:

@observer class Parent extends Component { store = observable({ get cmp() { return this.props.foo + 5; } }) render() { return <Child store={store} /> } } @observer class Child extends Component { render() { return store.cmp; } }

@urugator I see what you mean now
yes this is an unintended "feature" that let child display "correct" value, therefore I feel removing it will cause unexpected issue in our fairly large codebase

that props must be observable for every derivation, except for observer derivation - that is any user defined computed/reaction/autorun.

This is something I can do, as per this commit, I can change globalState.trackingDerivation to be an array, and add isReactObserver(name TBD) to IDerivation

Eventually, during props' get method, I can check if other react observer is tracking it and show a error in console

jzhan-canva added 2 commits April 26, 2024 14:17

reintroduce observable state,props,context

a38b459

test and readme

617bb35

jzhan-canva added 2 commits April 26, 2024 14:36

Create bright-hornets-listen.md

af3d709

add tests back

7299b08

add test

6ff48f3

jzhan-canva commented May 23, 2024

View reviewed changes

jzhan-canva changed the title ~~Reintroduce observable state,props,context in mobx-react~~ Reintroduce observable state,props,context in mobx-react. solve tearing May 24, 2024

mweststrate mentioned this pull request May 29, 2024

[mobx-react] Maximum update depth exceeded error since mobx-react@8 #3865

Open

jzhan-canva changed the title ~~Reintroduce observable state,props,context in mobx-react. solve tearing~~ Solve tearing in class component,.Reintroduce observable state,props,context in mobx-react. Jun 4, 2024

jzhan-canva changed the title ~~Solve tearing in class component,.Reintroduce observable state,props,context in mobx-react.~~ Solve tearing in class component. Reintroduce observable state,props,context in mobx-react. Jun 4, 2024

jzhan-canva force-pushed the mobx-react-observable-props branch from 1840137 to 6ff48f3 Compare June 4, 2024 09:10

jzhan-canva changed the title ~~Solve tearing in class component. Reintroduce observable state,props,context in mobx-react.~~ Reintroduce observable state,props,context in mobx-react Jun 4, 2024

jzhan-canva closed this Jun 4, 2024

add shouldReportChanged

3a8c951

jzhan-canva reopened this Jun 4, 2024

urugator reviewed Jun 4, 2024

View reviewed changes

jzhan-canva requested a review from urugator June 14, 2024 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce observable state,props,context in mobx-react #3863

Reintroduce observable state,props,context in mobx-react #3863

jzhan-canva commented Apr 26, 2024 •

edited

Loading

changeset-bot bot commented Apr 26, 2024 •

edited

Loading

jzhan-canva commented May 7, 2024

urugator commented May 7, 2024

jzhan-canva commented May 8, 2024 •

edited

Loading

jzhan-canva commented May 23, 2024 •

edited

Loading

jzhan-canva commented May 23, 2024 •

edited

Loading

jzhan-canva May 23, 2024

jzhan-canva commented Jun 4, 2024 •

edited

Loading

jzhan-canva commented Jun 4, 2024 •

edited

Loading

mweststrate commented Jun 4, 2024

urugator Jun 4, 2024

urugator Jun 4, 2024

urugator Jun 4, 2024 •

edited

Loading

urugator Jun 4, 2024

jzhan-canva Jun 5, 2024

Reintroduce observable state,props,context in mobx-react #3863

Are you sure you want to change the base?

Reintroduce observable state,props,context in mobx-react #3863

Conversation

jzhan-canva commented Apr 26, 2024 • edited Loading

Code change checklist

changeset-bot bot commented Apr 26, 2024 • edited Loading

🦋 Changeset detected

jzhan-canva commented May 7, 2024

urugator commented May 7, 2024

jzhan-canva commented May 8, 2024 • edited Loading

jzhan-canva commented May 23, 2024 • edited Loading

jzhan-canva commented May 23, 2024 • edited Loading

jzhan-canva May 23, 2024

Choose a reason for hiding this comment

jzhan-canva commented Jun 4, 2024 • edited Loading

jzhan-canva commented Jun 4, 2024 • edited Loading

mweststrate commented Jun 4, 2024

urugator Jun 4, 2024

Choose a reason for hiding this comment

urugator Jun 4, 2024

Choose a reason for hiding this comment

urugator Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

urugator Jun 4, 2024

Choose a reason for hiding this comment

jzhan-canva Jun 5, 2024

Choose a reason for hiding this comment

jzhan-canva commented Apr 26, 2024 •

edited

Loading

changeset-bot bot commented Apr 26, 2024 •

edited

Loading

jzhan-canva commented May 8, 2024 •

edited

Loading

jzhan-canva commented May 23, 2024 •

edited

Loading

jzhan-canva commented May 23, 2024 •

edited

Loading

jzhan-canva commented Jun 4, 2024 •

edited

Loading

jzhan-canva commented Jun 4, 2024 •

edited

Loading

urugator Jun 4, 2024 •

edited

Loading